Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up whitespace in OTEL_RESOURCE_ATTRIBUTES #1541

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

inssein
Copy link

@inssein inssein commented Nov 3, 2023

Fixes #1540

Copy link

linux-foundation-easycla bot commented Nov 3, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

@mjallday mjallday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -87,6 +87,13 @@
_(configurator_resource_attributes).must_equal(expected_resource_attributes)
end
end

it 'cleans up whitespace in user provided resources' do
OpenTelemetry::TestHelpers.with_env('OTEL_RESOURCE_ATTRIBUTES' => 'important_foo=x, important_bar=y') do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this trim values as well?

Suggested change
OpenTelemetry::TestHelpers.with_env('OTEL_RESOURCE_ATTRIBUTES' => 'important_foo=x, important_bar=y') do
OpenTelemetry::TestHelpers.with_env('OTEL_RESOURCE_ATTRIBUTES' => ' important_foo=x, important_bar=y ') do

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this but decided against it - I could add it in, not sure what other otel instrumentation agents do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should be using Baggage format so should be trimming spance then URL decoding https://github.com/open-telemetry/opentelemetry-go/blob/main/sdk/resource/env.go#L92

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my first time ever writing ruby and I haven't been able to run the tests locally (got ruby 2 installed), but I have made the changes and hopefully they make sense.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait sorry I messed up the order, trim first! committing in a moment

@arielvalentin
Copy link
Contributor

@inssein checking in since it's been a while. Looks like the test suite is failing. Would you mind taking a look again?

@inssein
Copy link
Author

inssein commented Nov 21, 2023

@arielvalentin I took a quick look but I can't figure out how my test is wrong. I don't quite have a local development environment going, would love a pointer.

Edit: odd that it's only partial failures at the OS level.

@xuan-cao-swi
Copy link
Contributor

I suspect it's decode_www_form_component cause the cross-system difference. Could you try to remove URI.decode_www_form_component and see if it pass all tests?

And/Or use strip! instead of strip on the string?

@robbkidd
Copy link
Member

I'm suspecting there's also some intermittent flakiness in the tests. I started playing with the changes in this PR in my dev environment because I too could not figure out what was wrong. Tests pass most of the time, but error occasionally saying the two important_{foo|bar} attributes are missing from the expected resources.

Copy link
Contributor

github-actions bot commented Mar 1, 2024

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

@github-actions github-actions bot added the stale label Mar 1, 2024
@inssein inssein requested a review from kaylareopelle as a code owner March 2, 2024 20:16
@inssein
Copy link
Author

inssein commented Mar 2, 2024

Could I get permission to run workflows or does it have to be approved each time?

@arielvalentin
Copy link
Contributor

Could I get permission to run workflows or does it have to be approved each time?

The current policy is only for first time contributors. Once your first PR is merged, the policy will allow actions to run automatically for future PRs.

Thanks again for your understanding and your first contribution.

@github-actions github-actions bot removed the stale label Mar 3, 2024
@inssein
Copy link
Author

inssein commented Mar 3, 2024

When I run these tests locally via docker I get no failures:

opentelemetry-ruby git:(otel-resource-attributes-env) ✗ docker-compose run sdk bundle exec rake test
[+] Building 0.0s (0/0)                                                                                                                                                    docker:default
[+] Building 0.0s (0/0)                                                                                                                                                    docker:default
/app/sdk/test/opentelemetry/sdk/trace/span_test.rb:469: warning: assigned but unused variable - yielded_span
/app/sdk/test/opentelemetry/sdk/trace/tracer_test.rb:11: warning: already initialized constant SpanLimits
/app/sdk/test/opentelemetry/sdk/trace/span_test.rb:14: warning: previous definition of SpanLimits was here
Run options: --seed 37135

# Running:

..................................................................................................................................................................................................................................................................................

Finished in 2.043577s, 134.0786 runs/s, 238.7970 assertions/s.

274 runs, 488 assertions, 0 failures, 0 errors, 0 skips
Coverage report generated for Integration Tests to /app/sdk/coverage. 2494 / 2536 LOC (98.34%) covered.

I'm not a ruby developer and I don't quite understand what's going on. I can remove the uri decoding stuff but then it wouldn't be to spec.

Copy link
Contributor

github-actions bot commented Apr 3, 2024

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

@github-actions github-actions bot added the stale label Apr 3, 2024
@arielvalentin arielvalentin added keep and removed stale labels Apr 3, 2024
@arielvalentin
Copy link
Contributor

I think I understand why this is failing. The OTEL_RESOURCE_ATTRIBUTES are computed by a singleton Resources.default().

This test is likely running in a non-deterministic order and if the test runs first, the singleton is set with the appropriate values, however if another test sets the singleton then this test fails because it will not re-evaluate the environment value.

Instead of making this test part of the SDK configurator like you have it in here, I recommend using the Resource related tests for default, which is currently resetting the singleton value:

This will make your test suite stable and predictable while ensuring correctness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OTEL_RESOURCE_ATTRIBUTES parsing has whitespace in keys
5 participants